Remove the MQTT subsystem - #115
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe updater manifest source is now configurable and has guarded error handling. MQTT gateway ingestion, routes, services, protobuf definitions, Prisma models, dependencies, and related configuration are removed. Database migrations purge coordinates and drop MQTT-derived tables. ChangesUpdater manifest configuration
MQTT gateway removal
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UpdaterRoute
participant ManifestServer
Client->>UpdaterRoute: Request updater manifest
UpdaterRoute->>ManifestServer: Fetch configured manifest URL
ManifestServer-->>UpdaterRoute: Return response and JSON
UpdaterRoute-->>Client: Return manifest candidate or HTTP 502
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/updater.ts`:
- Around line 80-82: Update the manifest parsing flow around mostRecentManifest
so every invalid payload—non-array JSON, an empty array, or a first element that
is not a valid manifest object—returns HTTP 502 instead of reaching the existing
HTTP 500 or success responses. Replace the unsafe type assertion with runtime
validation using the existing manifest schema or equivalent manifest-object
check, while preserving the valid manifest path.
- Around line 59-68: In the non-OK branch of the manifest fetch handling, cancel
the response body with await manifestResponse.body?.cancel() before returning
the 502 response. Keep the existing logging and error response unchanged.
- Line 57: Update the manifest fetch in the updater request flow to pass an
explicit timeout signal, using AbortSignal.timeout only when the supported
production Node version is at least 16.14.0. Preserve the existing catch
handling so timeout rejections continue mapping to HTTP 502.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a86d829-858d-41bb-8f22-5150291ce216
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.env.examplebuf.yamlpackage.jsonprisma/migrations/20260811000000_purge_gateway_coordinates/migration.sqlprisma/migrations/20260811000100_drop_mqtt_tables/migration.sqlprisma/schema.prismaprotobufs/gateway/v1/gateway.protoprotobufs/gateway/v1/gateway_service.protosrc/index.tssrc/lib/index.tssrc/lib/mqtt.tssrc/lib/prisma.tssrc/routes/index.tssrc/routes/mqtt.tssrc/routes/updater.tssrc/services/gateway.tssrc/services/index.tssrc/utils/yieldFromEvent.ts
💤 Files with no reviewable changes (12)
- src/services/index.ts
- protobufs/gateway/v1/gateway_service.proto
- buf.yaml
- src/routes/index.ts
- src/services/gateway.ts
- protobufs/gateway/v1/gateway.proto
- src/routes/mqtt.ts
- src/lib/mqtt.ts
- src/lib/prisma.ts
- src/utils/yieldFromEvent.ts
- src/lib/index.ts
- src/index.ts
| let parsedManifests: unknown; | ||
|
|
||
| try { | ||
| const manifestResponse = await fetch(manifestUrl); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: the declared runtime supports the chosen fetch timeout API.
rg -n -C 3 'node-version|engines|FROM node|AbortSignal\.timeout|AbortController' . || trueRepository: meshtastic/api
Length of output: 35198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(package\.json|.*Dockerfile.*|.*ya?ml|.*json|src/routes/updater\.ts)$' | head -200
printf '%s\n' '--- updater outline ---'
ast-grep outline src/routes/updater.ts --view expanded
printf '%s\n' '--- updater implementation ---'
cat -n src/routes/updater.ts
printf '%s\n' '--- declared runtime/config references ---'
rg -n -C 4 --glob 'package.json' --glob 'Dockerfile*' --glob '*.yml' --glob '*.yaml' \
'engines|node-version|FROM node|runtime|AbortSignal\.timeout|AbortController|fetch\(' . || trueRepository: meshtastic/api
Length of output: 41706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package manifest ---'
cat -n package.json
printf '%s\n' '--- TypeScript configuration ---'
cat -n tsconfig.json
printf '%s\n' '--- CI runtime declarations ---'
cat -n .github/workflows/ci.yml
printf '%s\n' '--- runtime references outside dependency lockfile ---'
rg -n -C 3 --glob '!pnpm-lock.yaml' \
'node-version|engines|FROM node|NODE_VERSION|runtime|node:[0-9]|AbortSignal\.timeout|AbortController' . || trueRepository: meshtastic/api
Length of output: 17341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import re
from pathlib import Path
source = Path("src/routes/updater.ts").read_text()
package = json.loads(Path("package.json").read_text())
ci = Path(".github/workflows/ci.yml").read_text()
fetch = re.search(r"try\s*\{(?P<body>.*?)\}\s*catch\s*\(.*?\)\s*\{(?P<catch>.*?)\}", source, re.S)
assert fetch, "manifest try/catch not found"
assert "fetch(manifestUrl)" in fetch.group("body"), "manifest fetch is not inside try"
assert '.status(502)' in fetch.group("catch"), "catch does not return HTTP 502"
assert "AbortSignal.timeout" not in source and "AbortController" not in source, \
"source already contains a fetch timeout"
print("manifest fetch is inside the existing try/catch")
print("existing catch maps fetch/json rejection to HTTP 502")
print("no timeout API is present at the manifest fetch")
print(f"package engines declaration: {package.get('engines', '<absent>')}")
match = re.search(r'node-version:\s*["\']?([^"\'\s]+)', ci)
print(f"CI Node version declaration: {match.group(1) if match else '<absent>'}")
PYRepository: meshtastic/api
Length of output: 375
🌐 Web query:
Node.js AbortSignal.timeout API added version globals documentation
💡 Result:
The AbortSignal.timeout API was added to Node.js in versions v17.3.0 and v16.14.0 [1][2]. It is documented under the Global objects section of the official Node.js API documentation [1][3]. This static method creates and returns a new AbortSignal that automatically aborts after the specified number of milliseconds [1][4].
Citations:
- 1: https://nodejs.org/api/globals.html
- 2: https://nodejs.org/download/release/v22.12.0/docs/api/globals.html
- 3: https://nodejs.org/dist/latest/docs/api/globals.html
- 4: https://beta.docs.nodejs.org/globals.html
Bound the manifest fetch.
If the manifest source stalls, the updater request remains pending. Pass an explicit timeout signal to fetch; the existing catch already maps the rejection to HTTP 502. Use AbortSignal.timeout(...) only when the production Node version is at least 16.14.0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/updater.ts` at line 57, Update the manifest fetch in the updater
request flow to pass an explicit timeout signal, using AbortSignal.timeout only
when the supported production Node version is at least 16.14.0. Preserve the
existing catch handling so timeout rejections continue mapping to HTTP 502.
Ingest has not processed a packet since 2024-07-15. The topic parser sliced at a fixed offset (`topic.substring(8)`), which only matched the pre-2.3 layout, so every region-prefixed topic fell through to "Unknown topic". With no connect or error handler on the client, a broker that never matched looked identical to one that was working. Meanwhile GET /mqtt and the gatewayStream RPC served the Gateway table unauthenticated, latitude and longitude included at full precision. Those coordinates were never the gateway's own position: the row is keyed on packet.gatewayId, but the coordinates came from the POSITION_APP payload of the relayed packet, whose origin is packet.packet.from. Each row held the last known position of some other node the gateway happened to forward -- nodes whose owners never chose to publish anything -- and there is no retention logic anywhere in the repo. Rather than repair ingest and resume collecting that data, remove the subsystem: - delete the ingest, the /mqtt route and the gatewayStream service - drop the Gateway and Channel models, the only two in the schema, and with them the ConnectRPC layer and the gateway protobufs that existed solely to serve them - remove nine now-unused dependencies Two migrations run on deploy: the first nulls the stored coordinates, the second drops both tables. The purge is redundant once the drop succeeds and is kept deliberately, because Prisma halts on the first failing migration -- if the DROP fails, the coordinates have already been cleared. Prisma is retained for this release only, as the mechanism that runs `prisma migrate deploy` from the start script; removing it in the same change would mean the drop never reaches production. The Gateway and Channel models stay in the schema alongside it, because `prisma generate` exits 1 on a model-less schema and the build runs it -- so for one release the schema deliberately describes two tables that no longer exist. Nothing reads them; no code imports the generated client. Follow-up steps are noted in schema.prisma. Also replaces the hardcoded gist in the updater route with UPDATER_MANIFEST_URL, and returns 502 rather than throwing out of the handler when the manifest source is unreachable.
c81a1c3 to
968c5b6
Compare
Three issues in the manifest handling, from review: - The fetch had no timeout, so a manifest source that accepted the connection and then stalled left the updater request pending indefinitely. It now aborts after 10s, and the rejection maps to 502 like any other upstream failure. - The non-ok branch returned without consuming the response body, which holds the connection open until it is garbage collected. Cancel it before returning. - `parsedManifests[0] as object | undefined` asserted a shape rather than checking it, so an array whose first element was a string passed the truthiness guard and was served to clients with a 200. The payload is now validated at runtime, and every shape this endpoint cannot use -- non-array JSON, an empty array, a non-object first element -- returns 502 rather than 500, since the fault lies with the manifest source rather than with this service. Verified against a stub manifest source: a valid array returns 200 with the first manifest; a string first element, a null first element, a non-array object, an empty array, a non-JSON body and an upstream 404 all return 502; a stalling source returns 502 after the 10s timeout.
1 participant
Why
Two problems with a shared root.
GET /mqttreturned stored position data to anyone. An unauthenticated request returned the fullGatewaytable includinglatitudeandlongitudeat full precision, and the same data went out over thegatewayStreamRPC. As of 11 Aug 2026 that was 2,692 gateway rows, 847 of them carrying coordinates, written between January and July 2024 and never deleted — there is no retention logic anywhere in the repo.The coordinates were never the gateway's own position. The row is keyed on
packet.gatewayId, but the lat/lon came from thePOSITION_APPpayload of the packet being relayed, whose origin ispacket.packet.from. So each row held the last known position of some other node that gateway happened to forward — nodes whose owners never chose to publish anything. Channel names went out alongside them, and some of those identify people.The CORS whitelist was not a control here:
origin(req)returns""when there is noOriginheader, so any non-browser client passed straight through.The ingest had been dead since 2024-07-15.
topic.substring(8).split("/")only decoded when the result had exactly two parts, which matches the oldmsh/2/c/<channel>/<node>layout. Region-prefixed topics —msh/EU_868/2/e/<channel>/<node>, standard since firmware 2.3/2.4 — split into four or five parts and fell through toconsole.log("Unknown topic"). It went unnoticed for thirteen months because there was noconnectorerrorhandler on the client: a broker that never matches looks identical to one that is working.Repairing the parser would have resumed collecting the same wrongly-attributed location data, so the subsystem is removed instead.
What this removes
/mqttroute, and thegatewayStreamserviceGatewayandChanneltables — the only two in the schema (models stay declared for one release, see below)GatewayServicewas the only registered service) and the gateway protobufs that existed solely to serve itutils/yieldFromEvent.ts, written for the gateway stream and referenced only from a commented-out block inside itmqtt,@meshtastic/js,@bufbuild/protobuf, both@connectrpc/*, both@buf/meshtastic_api.*,@prisma/client,sub-eventsDeploying this — please read
This needs two deploys, in order.
prisma migrate deployin thestartscript is what runs the table drop. Removing Prisma in the same change would mean the drop never executes, leaving the coordinates in Postgres with no code path left to reach them. So Prisma stays for exactly one release as the migration runner.Why the schema still declares
GatewayandChannelafter the migrations drop them:prisma generateexits 1 on a schema with no models, and the build runs it (build=prisma generate && tsc). Emptying the schema here would fail the Railway build, and the drop would never run. So the build config in this PR is byte-identical to the one deploying successfully today, and for one release the schema describes two tables that no longer exist. Nothing reads them — no code imports the generated client. Don't runprisma migrate devagainst a deployed database in that window; it would read the missing tables as drift and try to recreate them.startapplies20260811000000_purge_gateway_coordinates, then20260811000100_drop_mqtt_tables. The drop is irreversible — it destroys the 847 coordinate rows and the 4,307 channel stat rows with them.schema.prisma: delete the schema and migrations directory, drop theprismadevDependency and themigrate deploystep, and removeDATABASE_URL.DATABASE_URLmust stay set on Railway until step 1 has run.The
prismaCLI is a devDependency and thestartscript already depends on it today, so this relies on Railway keeping devDependencies present at start — which it does now, or the current deployment would already be failing. Worth a glance at the deploy log to confirm the two migrations apply.Breaking
GET /mqttnow returns 404.POST /meshtastic.api.gateway.v1.GatewayService/GatewayStreamnow returns 404.map.meshtastic.orgis CORS-whitelisted and is the likeliest consumer — worth telling whoever owns it rather than letting them discover it.buf.build/meshtastic/api; anything generating from that module keeps working until someone pushes.MQTT_URL/MQTT_USERNAME/MQTT_PASSWORD/MQTT_ROOT_TOPICare unused now and should be removed from the Railway environment. The broker credentials are worth rotating.map.meshtastic.organdConnect-Protocol-Versionare left in the CORS config on purpose — both are harmless, and dropping the origin could break the firmware and resource routes.Verification
Migrations were applied to a throwaway Postgres container seeded to resemble production:
purgeupdatedAtpreserved at 2024-07-15drop_prisma_migrationsremainsupdatedAtsurvives the purge by design — Prisma maintains it client-side with no database trigger, so the rawUPDATEleaves the record of when each row was last written.Also:
pnpm install --frozen-lockfileclean,pnpm build(prisma generate && tsc) exits 0, Biome reports zero warnings (the two onmasterwere in deleted files), and no references to mqtt, gateway or connectrpc remain insrc/. Booting the compileddist/index.js— which is whatstartruns:/→ 200,/updater→ 200,/mqtt→ 404,gatewayStream→ 404.pnpm startwas deliberately not run end-to-end locally, since it would invokeprisma migrate deployand that must not reach production ahead of the deploy. The migration step was verified separately against the throwaway container above, andnode dist/index.jsseparately on its own.Also in here
routes/updater.tsfetchedgist.githubusercontent.com/ajmcquilkin/…/manifests.jsonon every request — a production endpoint depending on one person's personal gist. The source is nowUPDATER_MANIFEST_URL, falling back to the gist so nothing breaks, and an unreachable or non-JSON manifest returns 502 instead of throwing out of the handler. Pointing it at infrastructure the project controls is still worth doing.Unrelated, found while testing
The server does not boot on Node 25:
@octokit/auth-app→jsonwebtoken→jwa→buffer-equal-constant-time@1.0.1readsbuffer.SlowBuffer, which Node 25 removed. Pre-existing and independent of this change —node -e "import('@octokit/auth-app')"fails the same way on its own. CI does not catch it becausebuildis onlytsc, which never imports the package, and CI pinsnode-version: "latest". Not addressed here.